feat(forge): Add semconv_const filter to support semantic convention namespacing rules.
#200
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
=====================================
Coverage 74.3% 74.4%
=====================================
Files 44 44
Lines 2921 2936 +15
=====================================
+ Hits 2172 2185 +13
- Misses 749 751 +2 ☔ View full report in Codecov by Sentry. |
| /// to the specified case convention. | ||
| pub fn str_to_case_converter(case_convention: &str) -> Result<fn(&str) -> String, Error> { | ||
| match case_convention { | ||
| "lower_case" => Ok(lower_case), |
There was a problem hiding this comment.
not strictly related to this PR, do we really need helpers for upper|lower|title - they are supported by jinja directly
There was a problem hiding this comment.
For consistency, the idea was to align all the case converters with the same naming pattern: <mode>_case.
There was a problem hiding this comment.
It's not a strong opinion, but I'd rather not redefine filters provided by jinja out of the box.
| "snake_case" => Ok(snake_case), | ||
| "screaming_snake_case" => Ok(screaming_snake_case), | ||
| "kebab_case" => Ok(kebab_case), | ||
| "screaming_kebab_case" => Ok(screaming_kebab_case), |
There was a problem hiding this comment.
do we need kebab case and it's variations for constant names?
There was a problem hiding this comment.
I can remove them, but I was wondering if there are programming languages with this type of convention for declaring constants. None come to mind, but perhaps they exist?
There was a problem hiding this comment.
Ok we probably don't have an SDK for Lisp, but in Common Lisp constants are following the kebab-case pattern.
There was a problem hiding this comment.
it seems Closure also allows -. I don't mind having a specific method for kebab_case constant (maybe not the screaming one though?)
| @@ -24,6 +25,7 @@ pub(crate) fn add_tests_and_filters(env: &mut minijinja::Environment<'_>) { | |||
| env.add_filter("not_required", not_required); | |||
| env.add_filter("instantiated_type", instantiated_type); | |||
| env.add_filter("enum_type", enum_type); | |||
| env.add_filter("semconv_const", semconv_const); | |||
There was a problem hiding this comment.
since most of the cases don't make sense in the context of semconv_const (and we only need them as convenience), I'd prefer to have a filter per class of constants:
pascal_case_constcamel_case_constsnake_case_constscreaming_snake_case_const
That's all we have in build-tools and it seems it was sufficient for all languages.
There was a problem hiding this comment.
I can follow this pattern if it's already the existing approach (I thought these filters didn't exist, so I didn't check).
There was a problem hiding this comment.
they are called differently: https://github.com/open-telemetry/build-tools/blob/main/semantic-conventions/src/opentelemetry/semconv/templating/code.py#L166-L181
but my main point is that semconv_const(name, "title") does not make sense, so instead of allowing someone to use it wrong with arbitrary case value, we can provide specific methods.
There was a problem hiding this comment.
Changed the code to follow your proposal.
See open-telemetry/semantic-conventions#1118
And #198
Closing: #198